Editorial: Add early return in CalendarDateUntil #3218
Conversation
catamorphism
force-pushed
the
remove-check-untilCalendar
branch
from
b380ac7 to
91056e5
Compare
catamorphism
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3218 +/- ##
==========================================
+ Coverage 96.75% 97.88% +1.12%
==========================================
Files 22 22
Lines 10398 10372 -26
Branches 1859 1808 -51
==========================================
+ Hits 10061 10153 +92
+ Misses 289 199 -90
+ Partials 48 20 -28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ptomato
left a comment
ptomato
left a comment
There was a problem hiding this comment.
I found a code snippet that triggers this assertion:
const d1 = Temporal.PlainDateTime.from('2026-01-06T11:02[u-ca=gregory]');
const d2 = Temporal.PlainDateTime.from('2026-01-07T09:02[u-ca=gregory]');
d1.until(d2, { largestUnit: 'years' });So probably we should consider it a testing coverage gap in any case.
I guess to fix this, we should decide at what level we want to have this early return. As long as it isn't observable, we can do this editorially.
One possibility is to fix all callers of CalendarDateUntil both in the polyfill and spec text so that we can make the assertion in CalendarDateUntil. Another possibility is to just make CalendarDateUntil definitively return if the two dates are equal and then we can make the assertion here in nonISOHelperBase.untilCalendar.
@ptomato I added the early return in CalendarDateUntil; that seems like the simplest thing. I'll also submit a test262 PR with your test. |
|
test262 PR: tc39/test262#4813 |
ptomato
left a comment
ptomato
left a comment
There was a problem hiding this comment.
Looks good now, but could you make the same change to CalendarDateUntil in the spec text?
This change makes it possible to change a check to an assertion in the polyfill.
catamorphism
force-pushed
the
remove-check-untilCalendar
branch
from
89ae04b to
09880a9
Compare
Done. Squashed and rebased so I could update the PR title to "Editorial: ..." |
catamorphism
changed the title
| 1. If _calendar_ is *"iso8601"*, then | ||
| 1. Let _sign_ be -CompareISODate(_one_, _two_). | ||
| 1. If _sign_ = 0, return ZeroDateDuration(). | ||
| 1. Set _sign_ to -sign. |
There was a problem hiding this comment.
This is a bit odd-looking but I'm not sure how else to do it that would make sense.
Co-authored-by: Philip Chimento <[email protected]>
Co-authored-by: Philip Chimento <[email protected]>
As of tc39#3218 , this check is always done before calling dateUntil.
As of #3218 , this check is always done before calling dateUntil.
2 participants
This change makes it possible to change a check to an assertion in the polyfill.